Backport Sass from upstream #443
Conversation
XhmikosR
force-pushed
the
xmr/scss
branch
2 times, most recently
from
7b17702 to
e991979
Compare
src/assets/scss/_ads.scss
Outdated
| padding: 12px 12px 12px 154px; | ||
| margin: 1.5rem auto 0; | ||
| padding: 15px 15px 15px 160px; | ||
| margin: 2rem 0; |
There was a problem hiding this comment.
Maybe the surrounding HTML code is different in the blog, but margin: 2rem 0; causes a regression making the ad not horizontally centered at some breakpoints:
The easiest fix would be to do margin: 2rem auto 0; instead.
If we want exactly the same Scss file, we should rather dig into the HTML structure surrounding the ad "component".
There was a problem hiding this comment.
Feel free to push your changes as always
There was a problem hiding this comment.
What's your preferred approach? Having exactly the same Sass file for an easier maintenance?
There was a problem hiding this comment.
I took time to dig into this topic in detail and pushed 5febd28 to fix the issue.
We've gathered upstream _ad.scss as is. However, upstream, there's an extra rule for #carbonads in site/assets/scss/_masthead.scss:
#carbonads { // stylelint-disable-line selector-max-id
margin-inline: auto;
}Ads are displayed in different contexts upstream so that an extra rule is not always needed. For the blog, it must be used. So I've added the extra auto directive and added a comment to try to show that this is specific.
You can remove or reword this comment the way you think it's going to be the most helpful in terms of maintenance; our future selves should understand at some point the following:
- blog:
_ad.scss= upstream:_ad.scss+ extraautorule.
There was a problem hiding this comment.
FYI, some classes defined in this document are not used in the blog, such as .custom-tooltip and .custom-popover.
There was a problem hiding this comment.
Yeah, but I'd rather keep the used files as closed as possible. Alternatively, we can comment out the unused crap and add a comment
There was a problem hiding this comment.
Got no problem with that, just wanted to spot it in case you weren't aware of it.
|
@julien-deramond do you want to make any further changes here? |
There was a problem hiding this comment.
@julien-deramond do you want to make any further changes here?
Sorry for the delay, I was off for some time. I've checked the last modifications, I think we're good. Nice PR 👌
Preview: https://deploy-preview-443--bootstrapblog.netlify.app/